Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JENKINS-64291] Fix drag & drop issues replacing the drag & drop library #5177

Merged
merged 7 commits into from
Jan 22, 2021
Merged

[JENKINS-64291] Fix drag & drop issues replacing the drag & drop library #5177

merged 7 commits into from
Jan 22, 2021
+77 −170

Conversation

fqueiruga
Copy link
Contributor

@fqueiruga fqueiruga commented Jan 15, 2021 •
edited
Loading

See JENKINS-64291.

I haven't found a way to fix JENKINS-64291 other than straight up replacing the drag & drop library. This PR replaces the YUI drag & drop with https://github.com/SortableJS

Where to test:

  • JDK and Git configuration on Global Tools configuration

TODOs

  • Ensure changes work with all plugins
  • Move the JS code to webpack and import the library through NPM
  • Refactor the new CSS customization code
  • Remove the legacy dragdrop.js file and CSS code
  • Stop importing the dragdrop.js and the YUI dragdrop module
  • Enable AutoScroll
  • Test on IE11

Proposed changelog entries

  • Replace the drag & drop library used by Jenkins with SortableJS. This fixes the drag & drop functionality for the form changes.

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@timja
@oleg-nenashev
@Wadeck
@halkeye

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@fqueiruga fqueiruga changed the title [WIP- replace YUI dragdrop [WIP] - Fix drag & drop issues replacing the drag & drop library Jan 15, 2021
@fqueiruga fqueiruga marked this pull request as draft January 15, 2021 14:50
timja
timja reviewed Jan 15, 2021
View reviewed changes
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this functionally and I can't find any issues, so I vote let's replace the old yui library.

Cleanup needed here first before doing that though

Thanks!

@timja timja added the web-ui The PR includes WebUI changes which may need special expertise label Jan 15, 2021
@fqueiruga fqueiruga force-pushed the fix-drag-drop-regressions branch from ea52534 to 420bbac Compare January 18, 2021 16:43
@halkeye
Copy link
Member

halkeye commented Jan 19, 2021

I'm really excited for any removal of YUI. Lemme know how I can help.

@fqueiruga
Copy link
Contributor Author

Thank you Gavin. My manual testing so far has been successful. I did some code search across the plugin ecosystem and it seems some files may be sensible to these changes. I'd appreciate any help or indications for testing them out.

I'm listing only big profile plugins here

Pending clarifying these uses of custom elements, I think this PR is ready to accept reviews.

@fqueiruga fqueiruga changed the title [WIP] - Fix drag & drop issues replacing the drag & drop library [JENKINS-64291] Fix drag & drop issues replacing the drag & drop library Jan 19, 2021
@timja
Copy link
Member

timja commented Jan 19, 2021

There's 6 tests failing all seem to be for the same cause: https://github.com/jenkinsci/jenkins/pull/5177/checks?check_run_id=1727548397

@fqueiruga
Copy link
Contributor Author

It may have to do with the removal of the YUI drag & drop module, that it affects the YUI container resizing

@fqueiruga fqueiruga requested a review from daniel-beck January 19, 2021 14:44
@fqueiruga
Copy link
Contributor Author

I restored the YUI dragdrop module to see whether it still loads.

@MRamonLeon
Copy link
Contributor

Can you keep drag&drop disabled while resizing enabled? To move forward removing YUI. Not sure whether YUI is structured such a way to allow that though.

@fqueiruga fqueiruga marked this pull request as ready for review January 20, 2021 09:20
@fqueiruga
Copy link
Contributor Author

TBH I don't know what the YUI resizing module even does, and I don't know how risky it would be to break it.

Given that the main purpose of this PR is a fix for an urgent issue, I'd suggest moving ahead without removing the load of that YUI dragdrop module. We can do that as part of a follow-up ticket.

@fqueiruga fqueiruga requested a review from halkeye January 20, 2021 11:03
escoem
escoem approved these changes Jan 20, 2021
View reviewed changes
Copy link
Contributor

@escoem escoem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested manually without issues.

@timja timja added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jan 20, 2021
@timja
Copy link
Member

timja commented Jan 20, 2021

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@timja timja merged commit fa1fa7e into jenkinsci:master Jan 22, 2021
@fqueiruga fqueiruga deleted the fix-drag-drop-regressions branch January 25, 2021 12:30
@daniel-beck
Copy link
Member

Caused https://issues.jenkins.io/browse/JENKINS-67496

@timja timja mentioned this pull request Dec 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@res0nance res0nance res0nance approved these changes

@timja timja timja approved these changes

@escoem escoem escoem approved these changes

@daniel-beck daniel-beck Awaiting requested review from daniel-beck

@halkeye halkeye Awaiting requested review from halkeye

Assignees
No one assigned
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@fqueiruga @halkeye @timja @MRamonLeon @daniel-beck @escoem @res0nance